Some comments.

Reviewed 39 of 39 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

lib/generators/USAGE, line 3 [r1] (raw file):
trailing spaces

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
Add named param prerender: false

lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 38 [r1] (raw file):
We're not using jquery by default

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
This makes some parts of the code expect that server rendering is on. This is definitely an issue.

This needs to be "" if not server rendering.

search code for server_bundle_js_file and you'll see

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 11 [r1] (raw file):
the function would not use props

also need to always pass the railsContext as the second param for components and stores

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
worth considering if we want this to be:

const store = ReactOnRails.getStore("myStore");

that way you can have multiple react components use the same store

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 22 [r1] (raw file):
line endings?

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
done

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 38 [r1] (raw file):
removing it completely.

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
Looks like actually a few lines of code on the backend will need to change that are tied to the words server/client.

Comments from Reviewable

Coverage decreased (-0.5%) to 84.832% when pulling 0ee40ba on feature/merge-server-client-webpack into 4ccc0ce on master.

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
yeah agreed.

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
@justin808 I dove more into the lib code and it seems it may be more complicated than we hoped. Right now the code is tied to the name's server for a bundle name and there are lines that are strictly tied to npm run build:server and npm run build:client.

response = `pgrep -fl 'bin/webpack\s(\\-w|\\-\\-watch)\s\\-\\-config\s.*#{type}.*\\.js'`

and

build_output = `cd client && npm run build:#{type}`

Now I think we have a few options.

1. Always require that the server bundle is also used in the client. Now there could be multiple generated bundles, but one of the client bundles would have to be used for the server as well.
2. Not sure if this will work but having npm run build:server and npm run build:client but they just pass variables to the same config to spit out different files. At that point it may make more sense to just go with 2 configs though.
3. we add a dynamic element that allows people to add on npm commands to do whatever like server render a separate config if needed.

I'm personally in favor of just going for a single config since most people will just be putting the entire bundle in their header which is awesome with turbolinks. And if you need to optmize, then you can still do vendor chunks as needed which would still play nice with all of this.

Comments from Reviewable

lib/generators/USAGE, line 3 [r1] (raw file):
Done.

Comments from Reviewable

Just changed the webpack compilation checks for the tests. Now it just checks to see if any stale files exist and then runs a new config variable we added if they do. we default to 'npm run build'.

Coverage increased (+0.4%) to 85.693% when pulling aa3d80f on feature/merge-server-client-webpack into 9995619 on master.

Reviewed 1 of 39 files at r1, 9 of 12 files at r2, 6 of 11 files at r3.
Review status: 40 of 48 files reviewed at latest revision, 8 unresolved discussions.

lib/react_on_rails.rb, line 17 [r3] (raw file):
Where did this go?

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
why remove these?

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
what if we just check for a "webpack -w" ?

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 11 [r1] (raw file):
Are you not finished with this one?

We should pass _props and _railsContext and use neither, as they will be used for store initialization.

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
Not done yet

lib/react_on_rails/test_helper.rb, line 57 [r3] (raw file):
Why are we removing this? Let's keep it.

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 49 [r3] (raw file):
Let's not remove all of this

Comments from Reviewable

Got some questions. Also Please see https://github.com/shakacode/react_on_rails/blob/master/docs%2Fcontributor-info%2Fcontributing.md. Notably, we need a changelog entry.

Review status: 40 of 48 files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable

@jbhatab Is this almost ready?

Reviewed 1 of 39 files at r1, 4 of 12 files at r2, 7 of 11 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

lib/react_on_rails.rb, line 17 [r3] (raw file):
We deleted it, it's super super brittle/unmaintainable and was not working anyway. Simply checking if the assets are up to date or not should be sufficient. There is only the unlikely case where a user changes something inside of the client code and is able to start the tests so fast that they kick off the ensure assets compiled process before the webpack process has finished compiling, in which case it just re-compiles so not that painful.

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
it isn't DRY and therefore is extremely brittle

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
we got rid of the process checker, this is a moot point now, right @jbhatab ?

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 11 [r1] (raw file):
see below comment

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
using the shared store is a very advanced, niche feature. I don't think we should generate it in a simple hello world example, as it's just going to confuse people. We should keep it as vanilla as possible and leave the advanced redux store feature for the API docs and possibly a recipe section of the docs.

lib/react_on_rails/test_helper.rb, line 57 [r3] (raw file):
See previous comment

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 49 [r3] (raw file):
this code is extremely brittle and not surprisingly was broken. Now that it's fixed, I highly suggest keeping it this way. It's much easier to read and works correctly now

Comments from Reviewable

Reviewed 26 of 39 files at r1, 10 of 12 files at r2, 8 of 11 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
why do we want to explicitly override the default the user sets in the config here? Isn't that a little assuming? It's also one additional thing they have to understand when first getting used to our API

Comments from Reviewable

lib/react_on_rails.rb, line 17 [r3] (raw file):
Yeah, that was the general logic. It was too fragile and the scenarios where it even mattered we're very rare.

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
Even if we do keep these, we would condense them down to build and build:dev vs having all of them, but I think we should keep all of the npm commands that are related to the client directory in the package.json client directory. This adds a layer of magic that may confuse new people into thinking it's necessary to use react_on_rails.

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
We can change this later if people complain. I think it's pretty minor.

Comments from Reviewable

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
Yup, we removed the process checker. It was very fragile and already brought up a bug where the core functionality of checking to see if the webpack file was stale broke. Also if people change their npm commands then it becomes useless.

Comments from Reviewable

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
@justin808 I want to add this in a separate PR or in the component generator PR. This PR is already getting a little big and we should merge this in first. I agree that it's a cool feature to show off.

Comments from Reviewable

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 22 [r1] (raw file):
show's a line ending for me.

Comments from Reviewable

Coverage increased (+0.09%) to 82.568% when pulling 5418c3b on feature/merge-server-client-webpack into 9491179 on master.

Coverage increased (+0.09%) to 82.568% when pulling 5418c3b on feature/merge-server-client-webpack into 9491179 on master.

Coverage increased (+0.09%) to 82.568% when pulling a26f399 on feature/merge-server-client-webpack into 9491179 on master.

Coverage increased (+0.09%) to 82.568% when pulling a26f399 on feature/merge-server-client-webpack into 9491179 on master.

Coverage increased (+0.05%) to 82.52% when pulling 540aa79 on feature/merge-server-client-webpack into 56c0c8d on master.

@jbhatab @robwise Please review my last commit. I'm going to create a beta release and test on the tutorial app.

@samnang, @jbhatab @robwise can you review a9f1f38

This is deployed live at http://www.reactrails.com.

Using: shakacode/react-webpack-rails-tutorial#287

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

lib/generators/USAGE, line 3 [r10] (raw file):

Comments from Reviewable

Reviewed 2 of 3 files at r9, 10 of 15 files at r11, 6 of 10 files at r12, 4 of 4 files at r13.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

lib/generators/USAGE, line 3 [r10] (raw file):

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 48 [r13] (raw file):

React on Rails will ensure your JavaScript generated files are up to date, using your
/client level package.json `build:test` command.

what's with the slash here, we want the line return preserved since this is a console message

lib/tasks/assets.rake, line 113 [r13] (raw file):

# puts "Enhancing assets:precompile with react_on_rails:assets:compile_environment"
# Rake::Task["assets:precompile"]
#   .clear_prerequisites

@justin808 did we ever figure out what clear_prerequisites does?

spec/dummy/config/initializers/react_on_rails.rb, line 23 [r13] (raw file):

  # If you are using the same file for client and server rendering, having this set probably does
  # not affect performance.
  config.server_bundle_js_file = "server-bundle.js"

shouldn't this be set to webpack-bundle.js by default, since by default we don't generate separate bundles?

spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb, line 16 [r10] (raw file):

lib/generators/react_on_rails/templates/base/base/client/.DS_Store, line 0 [r7] (raw file):
this is still in the commit, pls delete

Comments from Reviewable

Reviewed 1 of 7 files at r4, 5 of 8 files at r5, 5 of 16 files at r6, 2 of 5 files at r7.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

lib/generators/react_on_rails/templates/base/base/.DS_Store, line 0 [r13] (raw file):
.DS_Store

Comments from Reviewable

Review status: all files reviewed at latest revision, 13 unresolved discussions.

lib/generators/react_on_rails/base_generator.rb, line 97 [r6] (raw file):

Comments from Reviewable

Ready? I think so.

Review status: all files reviewed at latest revision, 10 unresolved discussions.

lib/generators/react_on_rails/base_generator.rb, line 97 [r6] (raw file):

lib/generators/react_on_rails/templates/base/base/.DS_Store, line [r13] (raw file):

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 48 [r13] (raw file):

lib/tasks/assets.rake, line 113 [r13] (raw file):

I documented in this file on why it's needed above.

spec/dummy/config/initializers/react_on_rails.rb, line 23 [r13] (raw file):

we didn't change that (no diff mark)

Comments from Reviewable